-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add feature to detect socketcand beacon #1687
Add feature to detect socketcand beacon #1687
Conversation
8e0552f
to
32f9fd3
Compare
Regarding the API, i'd suggest to leave the actual cfg_list: list[AutoDetectedConfig] = detect_beacon()
bus = SocketCanDaemonBus(**cfg_list[0]) A dictionary should contain all necessary keywords to instantiate the |
32f9fd3
to
2b869b2
Compare
I'll be testing this in a couple different environments, and will report back. |
Alright, tested it on both windows and linux. Good to go from my side. Please review for merge. |
All review comments addressed. |
You should mention it in the docs, or nobody will know about it 😄 |
Done. Edit ... docstring is there. Not familiar with the documentation on this project, and how it's built. Lemee look into it and see if I can make it pop up in the right place. Pointers appreciated in case anyone's looking. |
You need to add |
a0f5c74
to
7f7224b
Compare
Done. Docs are passing now. |
@zariiii9003 I've noticed sometimes tests will spuriously fail. I made an empty commit to trigger the tests and all are passing now. Let me know if any more changes are required to merge this. Appreciate the pointers in working with the doc system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few minor issues. I'll merge after you fix these. Thank you for your patience 👍
Co-authored-by: zariiii9003 <[email protected]>
Co-authored-by: zariiii9003 <[email protected]>
Hope I got them all. Thanks for your maintainership. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Hi All, Thanks in advance, greetings. |
This implements the client side service discovery for socketcand:
https://github.com/dschanoeh/socketcand/blob/master/doc/protocol.md#service-discovery
Need some feedback on how to enable this option. In the PR, I've proposed a way that makes the happy path as easy as possible ... though it is a breaking change for the SocketCanDaemonBus class.
Happy path:
If a host address (hostname,port) is provided, it works the same way as before.
If a host address (hostname,port) is not provided, it activates detect_beacon(). The user can optionally provide an address to listen for the beacon on a specific interface and port. If not provided, it listens on all interfaces at the default port 42000. If channel is None, it selects the first channel in the channel list provided in the beacon.